Extracting tags should be configurable based on the replacement. For backwards compatibility default should probably be not to extract; but I imagine you would prefer the other.

If the tag is used in the metric we should be able to configure to remove it from the dimension set. Default should probably be to remove.

We should talk about this. It's gonna be a bit tricky.

Should variables be an ImmutableMap? -- this also looks long enough to be chopped.

Sure, why not? Fixed.

I'm not nuts about the state sharing between this method and the caller.

Fixed

The matcher is for the metric pattern; what matches are you iterating over here? I am a little concerned you've combined two concepts.

As discussed. It kinda is, but it's also a fairly succinct way of specifying the intent.

• final
• What's the coverage this gives us?

Fixed. Coverage is 100%

Is \\ used to escape $ or should we just use $$? Either way, this needs to be documented in the class header.

Documented. Also note that \\ is just a java-ism for the literal '\'

I believe with most open-close style formats you need to escape both. I understand it's strictly not necessary here, but I'd like to be consistent with say regex and bash.

That's fair. Fixed.

chop

Fixed.

We should take this opportunity to document the mapping source in the README.md.

Thank you.

This should be ImmutableMap; no?

(and ImmutableList)

Fixed

List => ImmutableList

Fixed

Do you think we should eventually push the Key model class to this level? If so, can you add a TODO here? (if not, let me know what your thoughts are on it)

I migrated to using Key instead. Not too hard.

Can you rename this findAndReplacePattern?

The one thing I am not seeing here is how to select which metrics to apply which rules to. The difference between selecting a metrics and transforming it. While I realize that for metric name this can often be expressed with one regex, I suspect there is actually more to this pattern.

Specifically, I believe we'll need to provide "selection" of metrics based not only on name but also on dimension set. For example: contain key, containing key regex or containing key with value or regex of value; I don't see a point in just containing value or regex of value ... but that's probably my overly typed view of the world.

Implementing the full selection is not something you need to do now. However, I would like you to consider it in how the configuration is laid out. I would like to be able to add this without making non-backwards compatible changes to the configuration. To this end I believe we need to support a list of transformations. By default the transformation can match all, and then adding selectors can narrow those down. There is one question which is whether transformations are all applied or only some.

We can default either way, supporting the other can just be a flag away. Although I am concerned about how slippery the slope may be to a graph/tree representation (instead of a list). I would like to hear your thoughts on this one.

wrap one, wrap all

Fixed

nit: overwrite?

sure

Please add a TODO to refactor this into commons once there is a second use case for it.

There is no need to maintain compatibility with the mapping sink. I think that there are a few other cases here which may be handy:

${env:<name>}
${prop:<name>}
${capture:<name|number>}
${dimension:<name>}

While the simplicity of precedence is appealing, I cannot come up with a consistent scheme that doesn't break down. The namespacing while somewhat more arduous on users is explicit and easy to debug in case of failure (versus precedence which can result in unexpected results).

If you feel strongly another way I'd like to hear your thoughts on this.

Like regex capture groups, there should be no need to support escaped characters inside replacements unless you want to support dynamic replacements;

e.g.
${foo_${bar}}

This would certainly give insane flexibility.

Allowing escaping does not allow for dynamic replacements like that. It does allow for "special" characters in the names of the dimension, however. For example, if someone decided to have "{a}" as a dimension name, for whatever reason. Moreover, it makes for more consistent rules about how to escape (especially important if you have multiple levels of escaping.

Added the prefix support (env, capture, dimension are supported now). Precedence is explicit in the ordering of the prefixed variable maps. Phew, that was a bit of work.

I assume this is just on metric name?

Three points:

• It's unclear that the context of inject and remove is dimensions versus findAndReplace which is metrics.
• I can see that we'll want to apply findAndReplace to dimensions not just metrics in a future change.
• The find and replace is quite a bit more powerful than just a find and replace because it allows you to extract additional dimensions.

Suggestions:

• rename inject to injectDimensions
• rename remove to removeDimensions
• rename findAndReplace to transformMetric
• rename overwriteExisting to overwrite
• document the default value for overwrite

I'll look for this later, but I assume the semi-colon can be escaped if there is a literal one required.

In general, I am concerned that we're using an "expression" like language within in string instead of just specifying the control structure.

e.g.

transformMetric = [
    [
        match = "extract/([^/]*)/thing"
        replace = "extract/thing/${other_dimension}"
        injectDimensions = [
            my_dimension {
                value = "${1}"
                overwrite = true
             }
        ]
    ]
]

This is much more verbose, but there is less parsing, less code, and arguably less ambiguity.

I'll change the names. I don't think the semicolon can be escaped. I doubt that's something we'll really see, but I can add that functionality if you really feel strongly about it. Also, should I then be allowing escaping the '='?

I agree that your configuration example is more powerful (in most cases), but it adds significantly more processing (even after parsing is considered). If you want to add multiple dimensions, you need to run each dimension value through that same replacement, with the same context from the match. Also, you'll lose the ability to do something like "some/([^/]/([^/]/val;${1}=${2}" where you can have dynamic dimension keys. Moving the inject and remove dimensions into the metric transformation will likely be a significant code change for, IMHO, little value and significantly increased cost of use (configuration, processing time).

overwrite should have a default, probably true.

It does. Documented.

That seems somewhat surprising. Shouldn't it be kept, and then you can add it to remove if you want to drop it? I understand that's common behavior, but it's going to be harder to express using it and restoring it.

Removal of dimensions is not on a per-metric basis. The removal applies to all metrics. As for preserving it, it's as simple as "metric/${consumed};consumed=${consumed}". Basically, add it as a dimension with the same value.

Sorry, that's a weird scoping; why would the removal apply to all metrics but when the extraction only applies to some?

This is unclear and somewhat surprising since the rule only applies to a particular set of metrics based on the regex match.

Do they replace? What if I don't want them to?

If you didn't want them to replace, why would you put them there?

Indentation looks off.

Yeah.... wtf? Fixed.

Same comment about about numeric being something other than a capture group index and also the function calling this already special cased this.

^ This can also throw or be invalid if it overflows.

Fixed.

I think you only need to record variables that are dimensions. However, see my comments on the README about the default to remove.

The RegexAndMapReplacer doesn't know about the concept of dimensions. I was hoping to keep this class separate from the implementation details of the source.

Does the variableMap contain the environment variables too?

Only if you've added them to the variables Map<Map<String, String>>. But there is no special delineation here.

If you are intent on keeping the numeric representation, I think that you need to push it down to this level to ensure it's used in the correct precedence order.

Agreed. I didn't consider the case where an environment var, for example, was a number.

I'm not nuts about maintaining the internal data structures with the prefixes. The reason is that it means parsing and escaping the colons in more than one place. Have you considered just using the prefix to refer to a different input map? (e.g. map of maps where first key is prefix -- for example)

That's fair. I'll return that same map structure as gets passed in.

Type-o: Of => If

t => T

Instead of escaping, why not put it in ticks for inline code?

This is unclear and somewhat surprising since the rule only applies to a particular set of metrics based on the regex match.

I'm not convinced that the transformations should be treated as a single context. As a single context there can be only a single set of dimensions. In the case of say CollectD this is not necessarily the case. Consider cpu extracting core into a dimension; that doesn't apply to memory (were they in the same UOW). I think each transformation needs to create its own UOW from the existing one with the matching metrics and the dimensions that are elected. If we want to join them back (assuming dimension sets match) that's a possible optimization. Thoughts?

To elaborate:

• There is the incoming UOW
• Metrics matching a transformation are transformed into a new UOW which inherits its dimensions from the parent (post inject and remove) along with any injections/removals from the transform
• Any metrics included in a new UOW are removed from the parent UOW
• Metrics matching multiple UOWs are duplicated across them (subsequent changes adding separate matching rules to transformers can make this more elaborate)

That's essentially what we're doing with the merge map.

You should document that inject trumps remove and both of these trump anything extracted in the transform and subsequently removed. However, I'm a little concerned about the way this is done. Specifically, the way these can compound. Depending on your feedback on the independent UOWs from each transformation this may be a discussion we should have.